Add currency conversion support for BOLT 12 offers#3833
Add currency conversion support for BOLT 12 offers#3833shaavan wants to merge 10 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
|
cc @jkczyz |
|
🔔 1st Reminder Hey @joostjager! This PR has been waiting for your review. |
|
Is this proposed change a response to a request from a specific user/users? |
|
Hi @joostjager! This PR is actually a continuation of the original thread that led to the The motivation behind it was to provide users with the ability to handle So, as a first step, we worked on refactoring most of the Offers-related code out of Hope that gives a clear picture of the intent behind this! Let me know if you have any thoughts or suggestions—would love to hear them. Thanks a lot! |
|
Another use case is Fedimint, where they'll want to include their own payment hash in the |
Does Fedimint plan to use the |
I believe with one. |
|
🔔 2nd Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
Removing @joostjager for now to stop bot spam. @shaavan and I have been working through some variations of this approach. |
vincenzopalazzo
left a comment
vincenzopalazzo
left a comment
There was a problem hiding this comment.
Concept ACK for me
I was just looking around to sync with this Offer Flow
shaavan
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3833 +/- ##
==========================================
+ Coverage 89.34% 89.37% +0.02%
==========================================
Files 180 180
Lines 138480 140045 +1565
Branches 138480 140045 +1565
==========================================
+ Hits 123730 125164 +1434
- Misses 12129 12295 +166
+ Partials 2621 2586 -35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 8th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
Updated .13 → .14 Changes:
|
|
Rebased .14 → .15 |
|
🔔 12th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 13th Reminder Hey @jkczyz! This PR has been waiting for your review. |
InvoiceRequest::amount_msats previously mixed two meanings: the explicit request field and the payable amount inferred from the offer. That made call sites harder to reason about and obscured whether code was validating the request itself or the amount that should ultimately be paid. Make amount_msats return only the amount set directly on the request and remove the redundant has_amount_msats helper. Introduce payable_amount_msats for callers that need the effective amount derived from the request or offer, preserving explicit error reporting for missing, unsupported, or overflowing payable amounts. Co-Authored-By: OpenAI Codex <codex@openai.com>
Add a `CurrencyConversion` trait for resolving currency-denominated amounts into millisatoshis. LDK cannot supply exchange rates itself, so applications provide this conversion logic as the foundation for fiat-denominated offer support. Co-Authored-By: OpenAI Codex <codex@openai.com>
Add an internal MsatsRange type that carries a nominal millisatoshi amount together with its tolerated conversion range. This creates a single canonical result for resolving currency-denominated offer amounts without forcing callers to immediately collapse back to a plain u64. Keeping the nominal amount and tolerance together makes the later quantity-scaling and tolerant-comparison logic easier to express in one place. The new Amount::resolve_msats helper is the first consumer of this abstraction. It keeps the current behavior for bitcoin amounts while capturing the tolerance returned by CurrencyConversion for fiat amounts. Co-Authored-By: OpenAI Codex <codex@openai.com>
Add focused coverage for the new MsatsRange abstraction. The feature commit introduces a canonical range representation for resolved currency amounts, so this commit exercises the two key behaviors that later callers will rely on: deriving tolerated bounds from a conversion snapshot and scaling that range by quantity. It also moves TestCurrencyConversion into shared test utilities so later currency-conversion tests can reuse the same fixed USD rate and tolerance. Co-Authored-By: OpenAI Codex <codex@openai.com>
Store the CurrencyConversion implementation on ChannelManager and thread it through the ChannelManager constructor and read args. This keeps the conversion dependency with the higher-level payment state machine that owns offer payment state and persistence, while updating the background processor, block-sync setup, tests, and fuzz harnesses to construct ChannelManager with an explicit conversion implementation. Co-Authored-By: OpenAI Codex <codex@openai.com>
Allow OfferBuilder to accept currency-denominated amounts when the caller provides a CurrencyConversion implementation. Validate configured amounts in the builder setters instead of deferring that work to build(). That lets unsupported or invalid fiat amounts fail at the point they are provided and keeps build() focused on assembling and signing already-validated offer state. Co-Authored-By: OpenAI Codex <codex@openai.com>
|
🔔 14th Reminder Hey @jkczyz! This PR has been waiting for your review. |
Keep currency-dependent amount validation out of invoice request construction. Requests that omit amount_msats now keep the amount omitted on the wire. Request construction and parsing still enforce every amount check that is knowable locally, including missing-amount, quantity, bitcoin-denominated minimums, and max-value checks, while deferring minimum validation that requires currency conversion. This also drops the extra CurrencyConversion plumbing and related API changes from this commit so it stays focused on the request-side behavior change. Follow-up commits resolve and verify deferred amounts when handling invoices. Co-Authored-By: OpenAI Codex <codex@openai.com>
Extend invoice request parsing tests for currency-denominated offers to cover explicit serialized msat amounts. Verify that well-formed explicit amounts still parse and out-of-range values are rejected. Co-Authored-By: OpenAI Codex <codex@openai.com>
Use CurrencyConversion when turning an authenticated InvoiceRequest into a BOLT12 invoice and when validating the returned invoice on the payer. This keeps invoice_request.amount_msats omitted on the wire for currency-denominated offers while still letting the payee quote a concrete millisatoshi amount in the invoice. The same payable-amount logic is then reused to validate the authenticated invoice before it is recorded or paid. Payer-side amount validation failures are treated as local terminal errors. Invalid quoted amounts and unsupported local currency conversion now abandon the pending payment immediately instead of surfacing later as an invoice-request timeout, and they do not send InvoiceError back to the payee. Async and static invoice paths resolve offer amounts through the same conversion-aware builder so resolved amounts, payment secrets, and payment handling stay in sync. Co-Authored-By: OpenAI Codex <codex@openai.com>
Add tests for the deferred amount-validation model used by offer-backed BOLT12 invoices. The parsing coverage now exercises currency-denominated offers whose invoice requests omit amount_msats, confirming that the authenticated invoice may quote a concrete msat amount while the request remains omitted on the wire. The functional tests also cover the payer-side failure paths after invoice verification. An authenticated invoice with an out-of-range quoted amount and an authenticated invoice that requires unsupported local currency conversion both fail immediately with PaymentFailed and do not remain tracked as recent pending payments. Co-Authored-By: OpenAI Codex <codex@openai.com>
|
Updated .15 → .16 Changes:
|
|
cc @jkczyz |
| pub fn maximum_msats(&self) -> u64 { | ||
| let amount_msats = self.amount_msats as u128; | ||
| let maximum_msats = (amount_msats * (100 + self.tolerance as u128)).div_ceil(100); | ||
| u64::try_from(maximum_msats).expect("tolerance range cannot overflow") |
There was a problem hiding this comment.
Bug: maximum_msats() can panic when amount_msats * (100 + tolerance) exceeds u64::MAX * 100. This is reachable when checked_mul (line 1206) produces a large but valid u64 value combined with a non-zero tolerance.
For example: amount_msats = 18_200_000_000_000_000_000 (fits in u64), tolerance = 5 → maximum_msats = amount * 105 / 100 ≈ 19.1e18 > u64::MAX → panic.
This path is hit in production from channelmanager.rs:5803 via requested_amount.contains(invoice.amount_msats()), where the MsatsRange comes from payable_amount() → checked_mul(quantity). An attacker crafting a currency-denominated offer with a large amount could trigger this panic on the payer's node if the payer's exchange rate × offer amount × quantity is close to u64::MAX.
Consider using saturating arithmetic:
| pub fn maximum_msats(&self) -> u64 { | |
| let amount_msats = self.amount_msats as u128; | |
| let maximum_msats = (amount_msats * (100 + self.tolerance as u128)).div_ceil(100); | |
| u64::try_from(maximum_msats).expect("tolerance range cannot overflow") | |
| pub fn maximum_msats(&self) -> u64 { | |
| let amount_msats = self.amount_msats as u128; | |
| let maximum_msats = (amount_msats * (100 + self.tolerance as u128)).div_ceil(100); | |
| u64::try_from(maximum_msats).unwrap_or(u64::MAX) | |
| } |
| None => builder.amount_msats(1000).unwrap(), | ||
| Some(Amount::Bitcoin { amount_msats }) => builder.amount_msats(amount_msats + 1)?, | ||
| Some(Amount::Currency { .. }) => return Err(Bolt12SemanticError::UnsupportedCurrency), | ||
| Some(amount) => builder.amount_msats(amount.into_msats(&conversion)?)?, |
There was a problem hiding this comment.
Bug (won't compile): Amount has no into_msats method. The only conversion method is resolve_msats, which is pub(crate) and not accessible from the fuzz crate.
The old code matched on Amount::Bitcoin and Amount::Currency variants separately. The replacement needs a public conversion path. Possible fix: add a pub fn into_msats on Amount, or match on variants here as before:
| Some(amount) => builder.amount_msats(amount.into_msats(&conversion)?)?, | |
| Some(Amount::Bitcoin { amount_msats }) => builder.amount_msats(amount_msats + 1)?, | |
| Some(Amount::Currency { .. }) => return Err(Bolt12SemanticError::UnsupportedCurrency), |
| Err(Bolt12PaymentError::InvalidAmount) | ||
| | Err(Bolt12PaymentError::UnsupportedCurrency) => { | ||
| self.abandon_payment_with_reason( | ||
| $payment_id, PaymentFailureReason::UnexpectedError | ||
| ); | ||
| return None; |
There was a problem hiding this comment.
Bug: Double payment abandonment. When send_payment_for_verified_bolt12_invoice (line 5763-5768) encounters InvalidAmount or UnsupportedCurrency from check_bolt12_invoice_amount, it already calls abandon_payment_with_reason(payment_id, PaymentFailureReason::UnexpectedError) at line 5766 before returning the error. Then this macro arm calls abandon_payment_with_reason again.
The second call is effectively a no-op (the payment was already removed), but it's redundant and confusing. Either remove the abandon_payment call from send_payment_for_verified_bolt12_invoice (lines 5765-5767) and let the macro handle it, or don't match these error variants here (since they're already handled before the error propagates).
8 participants
This PR adds support for currency-denominated Offers in LDK’s BOLT 12 offer-handling flow.
Previously, Offers could only specify their amount in millisatoshis. However, BOLT 12 allows Offers to be denominated in other currencies such as fiat. Supporting this requires converting those currency amounts into millisatoshis at runtime when validating payments and constructing invoices.
Because exchange rates are external, time-dependent, and application-specific, LDK cannot perform these conversions itself. Instead, this PR introduces a
CurrencyConversiontrait which allows applications to provide their own logic for resolving currency-denominated amounts into millisatoshis. LDK remains exchange-rate agnostic and simply invokes this trait whenever a currency amount must be resolved.To make this conversion logic available throughout the BOLT 12 flow,
OffersMessageFlowis parameterized over aCurrencyConversionimplementation and the abstraction is threaded through the offer handling pipeline.With this in place:
OfferBuildercan now create Offers whose amounts are denominated in currencies instead of millisatoshis•
InvoiceRequesthandling can resolve Offer amounts when validating requests•
InvoiceBuilderenforces that the final invoice amount satisfies the Offer’s requirements after resolving any currency denominationCurrency validation is intentionally deferred until invoice construction when necessary, keeping earlier stages focused on structural validation while ensuring the final payable amount is correct.
Tests are added to cover the complete Offer → InvoiceRequest → Invoice flow when the original Offer amount is specified in a currency.